Fix open issues #4

liornoy · 2024-04-21T09:17:59Z

This PR fixes open issues.

Removes the unused e2etest and include the communication-matirx dir which holds the artifacts and the commatrix-gen binary. Signed-off-by: Lior Noy <[email protected]>

README.md

cmd/main.go

debug/debug.go

Change the flag passed to `os.OpenFile` from O_APPEND to O_TRUNC so each run will produce clean artifacts. Signed-off-by: Lior Noy <[email protected]>

Makefile

README.md

ss/ss.go

README.md

ss/ss.go

oribon · 2024-05-06T10:48:26Z

debug/debug.go

@@ -9,6 +9,7 @@ import (
 	"time"

 	corev1 "k8s.io/api/core/v1"
+	errutil "k8s.io/apimachinery/pkg/api/errors"


nit: import this as k8serrors / apierrors or just errors (errutil doesn't provide any meaning)

oribon · 2024-05-06T10:59:46Z

endpointslices/endpointslices.go

@@ -79,9 +79,7 @@ func ToComDetails(cs *client.ClientSet, epSlicesInfo []EndpointSlicesInfo) ([]ty
 // createEPSliceInfos gets lists of EndpointSlices, Services, and Pods and generates
 // a slice of EndpointSlicesInfo, each representing a distinct service.
 func createEPSliceInfos(epSlicesList *discoveryv1.EndpointSliceList, servicesList *corev1.ServiceList, podsList *corev1.PodList) ([]EndpointSlicesInfo, error) {


this should receive the slices of the relevant objects instead of the list ([]EndpointSlice,[]Service, []Pod). this is also relevant for all the other functions that don't care about the specific *List object (e.g getPod, getService)

oribon · 2024-05-06T11:02:42Z

endpointslices/endpointslices.go

+		} else {
+			svc = *service


nit: just deref service at the bottom, the "svc" var just confuses

fixed, thanks. I don't know why it didn't work for me when I first tried it.

oribon · 2024-05-06T11:11:48Z

ss/ss.go

@@ -100,6 +100,12 @@ func toComDetails(ssOutput []string, protocol string, node *corev1.Node) ([]type
 		if err != nil {
 			return nil, err
 		}
+		name, err := identifyContainerForPort(debugPod, ssEntry)


nit: it's not really clear what this function does (and the functions that it calls inside), e.g it says ForPort but receives an ssEntry. mind renaming them and/or adding some comments around them?

I re-worked the naming of the functions and added comments. let me know if this is better now.

oribon · 2024-05-06T11:15:53Z

ss/ss.go

+	ssOutFilteredTCP := filterStrings(filterOutFn, splitByLines(ssOutTCP))
+	ssOutFilteredUDP := filterStrings(filterOutFn, splitByLines(ssOutUDP))


nit: since both use the same filter, and filterStrings is only called here we can just rename filterStrings to something more meaningful and remove the filterFn semantics

done, thanks!

Utilize apimachinery errors package to check if error is of type AlreadyExists, and remove prior check that performs "Get". Signed-off-by: Lior Noy <[email protected]>

Removed unnecessary fields in getNamespaceDefinition function. Signed-off-by: Lior Noy <[email protected]>

Separate var block in the GetIngressEndpointSlicesInfo function. Signed-off-by: Lior Noy <[email protected]>

Reword function comment for clarity. Signed-off-by: Lior Noy <[email protected]>

This commit changes the functions `getPod` and `getService` to return only pointer instead val and bool, so that when it fails to find the required object it returns nil. Also made the required changes in createEPSliceInfos. Signed-off-by: Lior Noy <[email protected]>

Utilize identifyContainerForPort in `toComDetails` so that each ss entry will also include the container name. Also removed unused const `processeNameFieldIdx` Signed-off-by: Lior Noy <[email protected]>

sabinaaledort · 2024-05-09T10:09:44Z

README.md

-```
-
-#### Usage of EndpointSlice Resource
+### Usage of the EndpointSlice Resource

 This library leverages the EndpointSlice resource to identify the ports the  
 cluster uses for ingress traffic. Relevant EndpointSlices include those  
 referencing host-networked pods, Node Port services, LoadBalancer services,  
 or any custom EndpointSlice labeled with `"ingress":""`.


Where do we check for eps with "ingress":"" label?

Sorry, it's left over from the previous version. removed.

sabinaaledort · 2024-05-09T10:16:59Z

cmd/main.go

+		panic(err)
+	}
+	defer func() {
+		output, err := exec.Command("oc", "delete", "ns", consts.DefaultDebugNamespace).CombinedOutput()


Should we use the api instead of the exec command? Can be debug.DeleteNamespace

fixed, added the new function

sabinaaledort · 2024-05-09T11:04:20Z

cmd/main.go

+		if err != nil {
+			panic(err)
+		}
+	}()
 	nLock := &sync.Mutex{}


nit: missing new line

sabinaaledort · 2024-05-09T12:31:35Z

ss/ss.go


-	_, err = tcpFile.Write([]byte(fmt.Sprintf("node: %s\n%s", node.Name, strings.Join(ssOutFilteredTCP, "\n"))))
+	_, err = tcpFile.Write([]byte(fmt.Sprintf("node: %s\n%s", node.Name, string(ssOutTCP))))


Can you add an extra new line here? To separate between each node section

oribon · 2024-05-15T07:23:24Z

cmd/main.go

 	nLock := &sync.Mutex{}
 	g := new(errgroup.Group)
 	for _, n := range nodesList.Items {
 		node := n
 		g.Go(func() error {
-			cds, err := ss.CreateComDetailsFromNode(cs, &node, tcpFile, udpFile)
+			nLock.Lock()


why the lock here? this looks counter-productive (isn't the debug.New the thing we want to make concurrent since it waits for the pod to be created?)

I thought that we want to prevent 2 go routines from using the clientset together

Removed the lock.

oribon · 2024-05-15T07:25:29Z

commatrix/commatrix.go

-	}
-	comDetails = append(comDetails, add...)
+	comDetails = append(comDetails, MNOStaticEntries...)
+


nit: remove newline here, put it after the appends

oribon · 2024-05-15T08:22:46Z

ss/ss.go

 }

-func identifyContainerForPort(debugPod *debug.DebugPod, ssEntry string) (string, error) {
+// getContainerName receives an ss entry and gets the name of the container exposing this port.


nit: "ss entry" shows up a lot of times in this file, it would be useful to document how it looks like

added an example of ss entry to the README

This commit updates the README.md Signed-off-by: Lior Noy <[email protected]>

Fix the `go test` path in tasks.py. Signed-off-by: Lior Noy <[email protected]>

To fix lint check, added period in end of line of comments. Signed-off-by: Lior Noy <[email protected]>

This commit adds a check in the Makefile to see if the `oc` binary exist. if not, download and install it. Signed-off-by: Lior Noy <[email protected]>

This commit joins both filtering function for the ss entries, and rename it with a better name. Signed-off-by: Lior Noy <[email protected]>

This commit updates the static custom entries to be in struct format and not string. also add ports 111 and 53 for UDP. Signed-off-by: Lior Noy <[email protected]>

This commit modifies the functions respoinsible for extracting the container name of a given ss entry. - Renamed functions. - Added comments. - Changed function order. Signed-off-by: Lior Noy <[email protected]>

Since now the SS entries filter out functions for TCP and UDP are the same, here we modify the filterStrings function to include the filter out condition inside and remove the filterFn semantics. Signed-off-by: Lior Noy <[email protected]>

Modify `CreateComDetailsFromNode` function to write the raw-ss files before filtering the SS entries so that the file will reflect the real raw result. Signed-off-by: Lior Noy <[email protected]>

When using goroutines, the behavior of CreateComDetailsFromNode resulted in the deletion of the debug namespace, causing failure. Here, we relocate the creation and deletion of the debug namespace outside of the function to support running this function in parallel threads.

This commit changes the logic of `parseComDetail` function to log warning when can't extract service name, and set it to empty, instead of returning an error. This is to prevent the matrix generation from failing due to an `SS` entry without a service name like the following: `UNCONN 0 0 [::]:6081 [::]:* ` Signed-off-by: Lior Noy <[email protected]>

Added the service `ovn-kubernetes geneve` on port 6081 for MNO only, as part of the static-custom-entries.go. Signed-off-by: Lior Noy <[email protected]>

Fills fields for entires lacking information. Signed-off-by: Lior Noy <[email protected]>

This commit removes from the static custom enties the ports 9001 and 9192, which already exist as an endpointslice, and adds a call for remove duplications functions before returning the communcation matrix, to protect against future duplications. Signed-off-by: Lior Noy <[email protected]>

Update .gitignore

e624e92

Removes the unused e2etest and include the communication-matirx dir which holds the artifacts and the commatrix-gen binary. Signed-off-by: Lior Noy <[email protected]>

liornoy force-pushed the fix-open-issues branch from fefcc1d to 9a38214 Compare April 21, 2024 09:25

sabinaaledort reviewed Apr 24, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

sabinaaledort reviewed Apr 24, 2024

View reviewed changes

README.md Show resolved Hide resolved

sabinaaledort reviewed Apr 24, 2024

View reviewed changes

cmd/main.go Show resolved Hide resolved

sabinaaledort reviewed Apr 24, 2024

View reviewed changes

debug/debug.go Show resolved Hide resolved

sabinaaledort reviewed Apr 25, 2024

View reviewed changes

debug/debug.go Outdated Show resolved Hide resolved

Make the raw-ss files to truncate old files

32d85af

Change the flag passed to `os.OpenFile` from O_APPEND to O_TRUNC so each run will produce clean artifacts. Signed-off-by: Lior Noy <[email protected]>

liornoy force-pushed the fix-open-issues branch 2 times, most recently from 2b631b8 to b345759 Compare April 30, 2024 11:57

sabinaaledort reviewed May 1, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

sabinaaledort reviewed May 1, 2024

View reviewed changes

README.md Show resolved Hide resolved

sabinaaledort reviewed May 1, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

sabinaaledort reviewed May 1, 2024

View reviewed changes

ss/ss.go Outdated Show resolved Hide resolved

liornoy force-pushed the fix-open-issues branch 2 times, most recently from c1a2171 to fecb4f5 Compare May 2, 2024 11:12

sabinaaledort reviewed May 5, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

sabinaaledort reviewed May 5, 2024

View reviewed changes

ss/ss.go Outdated Show resolved Hide resolved

liornoy force-pushed the fix-open-issues branch from fecb4f5 to bb5718c Compare May 5, 2024 13:05

oribon reviewed May 6, 2024

View reviewed changes

liornoy added 4 commits May 6, 2024 16:42

Fix error handling in debug\createNamespace

7bed033

Utilize apimachinery errors package to check if error is of type AlreadyExists, and remove prior check that performs "Get". Signed-off-by: Lior Noy <[email protected]>

Clean fields in getNamespaceDefinition

0434c06

Removed unnecessary fields in getNamespaceDefinition function. Signed-off-by: Lior Noy <[email protected]>

Separate var declaration

2871bf3

Separate var block in the GetIngressEndpointSlicesInfo function. Signed-off-by: Lior Noy <[email protected]>

Reword function comment

4b6bb37

Reword function comment for clarity. Signed-off-by: Lior Noy <[email protected]>

liornoy force-pushed the fix-open-issues branch from bb5718c to f1c73da Compare May 8, 2024 10:15

liornoy added 2 commits May 8, 2024 13:24

Modify ss.go to include container name in matrix

ce6050f

Utilize identifyContainerForPort in `toComDetails` so that each ss entry will also include the container name. Also removed unused const `processeNameFieldIdx` Signed-off-by: Lior Noy <[email protected]>

liornoy force-pushed the fix-open-issues branch 2 times, most recently from 1b5ae23 to 088809e Compare May 8, 2024 14:36

liornoy mentioned this pull request May 8, 2024

Open issues #6

Closed

liornoy force-pushed the fix-open-issues branch from 088809e to 3f34cae Compare May 8, 2024 14:48

sabinaaledort reviewed May 9, 2024

View reviewed changes

liornoy force-pushed the fix-open-issues branch from 3f34cae to 34648eb Compare May 9, 2024 10:52

sabinaaledort reviewed May 9, 2024

View reviewed changes

liornoy force-pushed the fix-open-issues branch from 34648eb to 1b3c108 Compare May 9, 2024 12:18

sabinaaledort reviewed May 9, 2024

View reviewed changes

liornoy force-pushed the fix-open-issues branch from 92dbd5c to 8b00e33 Compare May 9, 2024 12:45

oribon reviewed May 15, 2024

View reviewed changes

liornoy added 13 commits May 15, 2024 16:30

Update README.md

491dd86

This commit updates the README.md Signed-off-by: Lior Noy <[email protected]>

Fix the test command

3b8e21b

Fix the `go test` path in tasks.py. Signed-off-by: Lior Noy <[email protected]>

Fix lint - add period for comments

4f1945e

To fix lint check, added period in end of line of comments. Signed-off-by: Lior Noy <[email protected]>

Check oc exist in Makefile

e64cc10

This commit adds a check in the Makefile to see if the `oc` binary exist. if not, download and install it. Signed-off-by: Lior Noy <[email protected]>

Modify the ss filtering functions

e199866

This commit joins both filtering function for the ss entries, and rename it with a better name. Signed-off-by: Lior Noy <[email protected]>

Update the static custom entries

09c5a17

This commit updates the static custom entries to be in struct format and not string. also add ports 111 and 53 for UDP. Signed-off-by: Lior Noy <[email protected]>

ss: improve readability

75541e5

This commit modifies the functions respoinsible for extracting the container name of a given ss entry. - Renamed functions. - Added comments. - Changed function order. Signed-off-by: Lior Noy <[email protected]>

Modify filterStrings function

78abf92

Since now the SS entries filter out functions for TCP and UDP are the same, here we modify the filterStrings function to include the filter out condition inside and remove the filterFn semantics. Signed-off-by: Lior Noy <[email protected]>

ss: Write raw ss output before filtering

3b36026

Modify `CreateComDetailsFromNode` function to write the raw-ss files before filtering the SS entries so that the file will reflect the real raw result. Signed-off-by: Lior Noy <[email protected]>

Add MNO only static entry - port 6081

fcf4448

Added the service `ovn-kubernetes geneve` on port 6081 for MNO only, as part of the static-custom-entries.go. Signed-off-by: Lior Noy <[email protected]>

Fix static-custom-entries

bb886a7

Fills fields for entires lacking information. Signed-off-by: Lior Noy <[email protected]>

liornoy force-pushed the fix-open-issues branch from df33ff6 to bb886a7 Compare May 15, 2024 13:33

sabinaaledort merged commit fb1b862 into openshift-kni:main May 21, 2024
1 check passed

liornoy deleted the fix-open-issues branch May 21, 2024 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix open issues #4

Fix open issues #4

liornoy commented Apr 21, 2024

oribon May 6, 2024

liornoy May 8, 2024

liornoy May 8, 2024

oribon May 6, 2024

liornoy May 8, 2024

oribon May 6, 2024

liornoy May 8, 2024

oribon May 6, 2024

liornoy May 8, 2024

oribon May 6, 2024

liornoy May 8, 2024

sabinaaledort May 9, 2024

liornoy May 9, 2024

sabinaaledort May 9, 2024

liornoy May 9, 2024

sabinaaledort May 9, 2024

sabinaaledort May 9, 2024

liornoy May 9, 2024

oribon May 15, 2024

liornoy May 15, 2024

liornoy May 15, 2024

oribon May 15, 2024

liornoy May 15, 2024

oribon May 15, 2024

liornoy May 15, 2024

		ssOutFilteredTCP := filterStrings(filterOutFn, splitByLines(ssOutTCP))
		ssOutFilteredUDP := filterStrings(filterOutFn, splitByLines(ssOutUDP))


		_, err = tcpFile.Write([]byte(fmt.Sprintf("node: %s\n%s", node.Name, strings.Join(ssOutFilteredTCP, "\n"))))
		_, err = tcpFile.Write([]byte(fmt.Sprintf("node: %s\n%s", node.Name, string(ssOutTCP))))

Fix open issues #4

Fix open issues #4

Conversation

liornoy commented Apr 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment